Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miscelaneous fixes and features: Set bugfixes, Conntrack zone support, FIB support #55

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

brizental
Copy link

@brizental brizental commented Jan 29, 2024

Folks, as I attempted to rewrite MozVPN Linux netfilter code in Rust I bumped into a few issues that are addressed in this PR.

  1. Set creation and nft_set macro bugs: 50915b6
  2. Conntrack zone support: f14e104
  3. FIB support: 3b60247

I also learned about missing definitions on libc, I'll send a PR up there at some point as well. You'll notice I used some hradcoded numbers here and that is the reason. For reference: rust-lang/libc#3566

The MozVPN PR is not up yet, but I'll link to it here once it is. Cheers!


This change is Reviewable

Beatriz Rizental added 4 commits January 29, 2024 13:50
I found two issues while attempting to create a set:
1. A straight forward bug on the ntf_set macro: expect call were called
   in non option or result types.
2. A not as straight forward issue, where all sets created had both the
   anonymous and constant flag. The anonymous one definitely should not
   be there, because we are creating a named set. The constant flag
   should be optional somehow. It doesn't really break my use case,
   but I didn't want it to be there, so I removed it.
Not really sure about this one, but  just followed what was done for the
other primitives.
Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for wanting to contribute! Sorry for the lack of response. This could get merged faster if it was split into separate PRs for separate fixes/features. I'll take a look at it per commit and see what I can maybe cherry-pick in.

@@ -45,6 +46,9 @@ impl Conntrack {
Conntrack::State => libc::NFT_CT_STATE as u32,
Conntrack::Status => libc::NFT_CT_STATUS as u32,
Conntrack::Mark { .. } => libc::NFT_CT_MARK as u32,
// TODO: Update this once libc has definitions for NFT_CT_ZONE
// (https://github.com/rust-lang/libc/issues/3566)
Conntrack::Zone { .. } => 17 as u32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Can you submit a PR to libc? We won't merge this with a hardcoded constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants